Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix snapshots missing iterations #3557

Closed
wants to merge 2 commits into from
Closed

Fix snapshots missing iterations #3557

wants to merge 2 commits into from

Conversation

JonathanPlasse
Copy link
Contributor

Until now the snapshots only contained the first iteration of diagnostics.
This PR adds the diagnostics from the other iterations.
The duplicates between the iterations are removed.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2023 •
edited
Loading

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     15.6±0.34ms     2.6 MB/sec    1.00     15.4±0.30ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      4.1±0.07ms     4.0 MB/sec    1.00      4.0±0.08ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.03   572.1±11.62µs     5.2 MB/sec    1.00   555.9±10.03µs     5.3 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.14ms     3.7 MB/sec    1.00      6.9±0.10ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.16ms     4.9 MB/sec    1.00      8.2±0.16ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1845.8±43.56µs     9.0 MB/sec    1.00  1848.3±40.19µs     9.0 MB/sec
linter/default-rules/numpy/globals.py      1.07    215.9±3.73µs    13.7 MB/sec    1.00    202.3±5.54µs    14.6 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.9±0.10ms     6.5 MB/sec    1.00      3.9±0.07ms     6.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     17.9±0.48ms     2.3 MB/sec    1.02     18.3±0.53ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.9±0.26ms     3.4 MB/sec    1.01      5.0±0.19ms     3.3 MB/sec
linter/all-rules/numpy/globals.py          1.02   629.2±23.47µs     4.7 MB/sec    1.00   616.0±23.12µs     4.8 MB/sec
linter/all-rules/pydantic/types.py         1.01      8.1±0.26ms     3.1 MB/sec    1.00      8.0±0.23ms     3.2 MB/sec
linter/default-rules/large/dataset.py      1.00      9.8±0.42ms     4.2 MB/sec    1.00      9.8±0.37ms     4.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00      2.1±0.07ms     7.8 MB/sec    1.02      2.2±0.08ms     7.6 MB/sec
linter/default-rules/numpy/globals.py      1.00   234.6±11.30µs    12.6 MB/sec    1.02    239.4±8.91µs    12.3 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.6±0.19ms     5.5 MB/sec    1.01      4.6±0.23ms     5.5 MB/sec

@JonathanPlasse JonathanPlasse changed the title Fix incomplete snapshots Fix snapshots missing iterations Mar 16, 2023
@JonathanPlasse JonathanPlasse force-pushed the fix-incomplete-snapshots branch from 5557c2a to 006a96f Compare March 16, 2023 21:01
@JonathanPlasse JonathanPlasse mentioned this pull request Mar 16, 2023
Merged
@charliermarsh
Copy link
Member

Hmm, this may have the potential to confuse, since the diagnostics that are generated after the first iteration will contain locations that don't map to the input file (assuming the input file is changed via autofix).

Is there an example rule or set of rules for which we missed something before, but have improved diagnostic coverage after this change?

@charliermarsh charliermarsh self-requested a review March 16, 2023 23:28
@JonathanPlasse
Copy link
Contributor Author

  • Yes, I separated in two commits Fix autofix conflict between D209 and D400 #3564, where there is the before and after snapshot where the former had three fixes compared to the latter that had only two.
  • The format of the snapshots, could be changed to contain an array of the iterations.

@JonathanPlasse JonathanPlasse force-pushed the fix-incomplete-snapshots branch from 006a96f to 838832a Compare March 17, 2023 07:21
@JonathanPlasse
Copy link
Contributor Author 

  ---
  source: crates/ruff/src/rules/pydocstyle/mod.rs
  expression: diagnostics
  ---
  - - kind:
        name: NewLineAfterLastParagraph
        body: Multi-line docstring closing quotes should be on a separate line
        suggestion: Move closing quotes to new line
        fixable: true
      location:
        row: 2
        column: 4
      end_location:
        row: 3
        column: 72
      fix:
        content: "\n    "
        location:
          row: 3
          column: 69
        end_location:
          row: 3
          column: 69
      parent: ~
    - kind:
        name: EndsInPeriod
        body: First line should end with a period
        suggestion: Add period
        fixable: true
      location:
        row: 2
        column: 4
      end_location:
        row: 3
        column: 72
      fix:
        content: "."
        location:
          row: 3
          column: 69
        end_location:
          row: 3
          column: 69
      parent: ~
- - - kind:
-       name: NewLineAfterLastParagraph
-       body: Multi-line docstring closing quotes should be on a separate line
-       suggestion: Move closing quotes to new line
-       fixable: true
-     location:
-       row: 2
-       column: 4
-     end_location:
-       row: 4
-       column: 8
-     fix:
-       content: "\n    "
-       location:
-         row: 4
-         column: 5
-       end_location:
-         row: 4
-         column: 5
-     parent: ~

@JonathanPlasse
Copy link
Contributor Author

I do not know why the CI is failing, the tests are passing locally.

@MichaReiser
Copy link
Member

MichaReiser commented Mar 17, 2023 •
edited
Loading

I do not know why the CI is failing, the tests are passing locally.

The failing tests are gated behind the logical_lines feature [source].

You can run them locally with

cargo insta test --all-features

@charliermarsh I wonder if we should remove the feature and instead rely on cfg(debug_assertions) or disable the feature for release builds stack overflow to avoid this confusion in the future.

@JonathanPlasse JonathanPlasse force-pushed the fix-incomplete-snapshots branch 3 times, most recently from 4163f04 to fb9e1f9 Compare March 17, 2023 09:19
@JonathanPlasse JonathanPlasse force-pushed the fix-incomplete-snapshots branch from fb9e1f9 to c2c8934 Compare March 20, 2023 22:53
@JonathanPlasse
Copy link
Contributor Author

@charliermarsh, do you want to merge this PR or should we close it?

@JonathanPlasse JonathanPlasse force-pushed the fix-incomplete-snapshots branch from c2c8934 to 9dc20ec Compare March 20, 2023 23:06
@charliermarsh
Copy link
Member

@JonathanPlasse - I do see the value, but thinking on it... I'd prefer to close, I'm just worried about making the snapshots more difficult to follow since they now represent a sequence of iterative changes. I'm sorry for not saying so sooner.

@JonathanPlasse
Copy link
Contributor Author

I understand, no problem. I could understand better how the tests work.

@charliermarsh charliermarsh mentioned this pull request Mar 21, 2023
Merged
@JonathanPlasse JonathanPlasse deleted the fix-incomplete-snapshots branch April 30, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charliermarsh charliermarsh Awaiting requested review from charliermarsh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JonathanPlasse @charliermarsh @MichaReiser